-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Note that DeserializingResourceReader should not be used with untrusted data #12198
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Note that DeserializingResourceReader should not be used with untrusted data #12198
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a security note to the DeserializingResourceReader class documentation to warn users that it should not be used with untrusted data, aligning it with the similar warning already present in the ResourceReader documentation.
- Replaces the placeholder "To be added" remarks with a proper security warning using an include file reference
- Adds markdown formatting structure to support the security note inclusion
xml/System.Resources.Extensions/DeserializingResourceReader.xml
Outdated
Show resolved
Hide resolved
|
FWIW, @jeffhandley, I'd prefer a bigger hammer than the piecemeal way we've been approaching this so far. When we run TM exercises (including the ones you've participated in!), recall that we typically prefer listing affirmative expectations and assumptions. We can't possibly guess at all the ways somebody will misuse our stuff, after all. :) If you look at https://learn.microsoft.com/en-us/dotnet/core/extensions/resources, it explicitly states:
This is a pretty clear indication that .NET expects resources to be part of the app deployment, which implies that they're on the same side of the trust boundary as the app code itself. The logical conclusion from this is that the .resx / .resources capabilities within .NET carry an implicit assumption that the payloads they're operating over will always be considered trustworthy. While it's straightforward enough to connect the dots and draw this conclusion oneself, IMO it would be helpful it if were explicitly stated somewhere centrally, perhaps at the https://learn.microsoft.com/en-us/dotnet/core/extensions/resources doc itself. We could consider a blurb along the lines of:
|
|
Thanks for that suggestion, @GrabYourPitchforks. I'll merge this PR and also follow up with one to github.com/dotnet/docs/blob/main/docs/core/extensions/resources.md to add that blurb. |
The ResourceReader docs cite that only trusted data should be used, but the DeserializingResourceReader Class does not include this note. While it should be implied, it's a good place to call it out.